Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Great Pool Migration (prep) #1954

Merged
merged 4 commits into from
Oct 27, 2022
Merged

Great Pool Migration (prep) #1954

merged 4 commits into from
Oct 27, 2022

Conversation

EndymionJkb
Copy link
Collaborator

Description

In preparation for propagating the leaner, meaner BasePool of ManagedPool fame to the rest of the code base, move it from the /vendor directory inside pool-weighted, and rename the old one LegacyBasePool (similar to how we did it when switching from token to BPT protocol fees).

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Dependency changes
  • Code refactor / cleanup
  • Documentation or wording changes
  • Other

Checklist:

  • [~] The diff is legible and has no extraneous changes
    (It got a bit confused in some places. And there is one extra line, to silence lint errors.)
  • Complex code has been commented, including external interfaces
  • Tests are included for all code paths (no test changes)
  • The base branch is either master, or there's a description of how to merge

Issue Resolution

Advances #1953

Copy link
Contributor

@nventuro nventuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'legacy' approach unfortunately makes this very hard to review, since git treats this as LegacyBasePool being a spinoff of the new BasePool, with the old stuff added back, and the new BasePool being a spinoff of the old one, with the old stuff removed.

I suggest we don't rename the new one to BasePool yet, and only do that once the legacy one has been deleted from the repo. Instead of calling the old one legacy and new one base, we could keep the old one and call the new one NewBasePool. Alternatively, we could rename both - but I'm not sure this will fix the diff issues.

@EndymionJkb
Copy link
Collaborator Author

EndymionJkb commented Oct 27, 2022

I suggest we don't rename the new one to BasePool yet, and only do that once the legacy one has been deleted from the repo. Instead of calling the old one legacy and new one base, we could keep the old one and call the new one NewBasePool. Alternatively, we could rename both - but I'm not sure this will fix the diff issues.

I tried it a couple ways; keeping the old one leads to a smaller diff at least.

@EndymionJkb EndymionJkb requested a review from nventuro October 27, 2022 15:15
@nventuro nventuro merged commit 21dc0f2 into master Oct 27, 2022
@nventuro nventuro deleted the legacy-base branch October 27, 2022 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants